-
Notifications
You must be signed in to change notification settings - Fork 147
JDK-8220222: specify clearly gradle project dependencies #401
Conversation
build.gradle
Outdated
@@ -1847,12 +1847,24 @@ project(":base") { | |||
|
|||
sourceSets { | |||
main | |||
shims | |||
test | |||
shims{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing space before {
. Same for all subsequent places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some specific feedback below, but otherwise looks fine. You will need to fetch the latest upstream changes and merge (not rebase) the upstream develop
branch into your branch to fix a trivial merge conflict.
build.gradle
Outdated
@@ -2396,13 +2431,14 @@ project(":swing") { | |||
//shims // no test shims needed | |||
test | |||
} | |||
|
|||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert this unwanted white-space change.
build.gradle
Outdated
project.ext.moduleSourcePath = defaultModuleSourcePath | ||
project.ext.moduleSourcePathShim = defaultModuleSourcePathShim | ||
|
||
commonModuleSetup(project, [ 'base', 'graphics', 'swing' ]) | ||
|
||
dependencies { | ||
compile project(":media") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
javafx.swing
does not depend on javafx.media
. I think this should instead be:
compile project(":base")
compile project(":graphics")
@@ -3119,6 +3179,9 @@ project(":web") { | |||
commonModuleSetup(project, [ 'base', 'graphics', 'controls', 'media', 'web' ]) | |||
|
|||
dependencies { | |||
compile project(":base") | |||
compile project(":graphics") | |||
compile project(":media") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should a dependency on controls
be added here as well?
…lop and itegrated code review javafxports#401 # Conflicts: # build.gradle
@kevinrushforth thanks for your review. You are right in all points and I integrated all your reviews and merged it with the develop branch of upstream. This checkin enables full working on OpenJFX in IntelliJ IDEA Version 20.18.3 or higher. The checked in IntelliJ project files should be removed, because they do not work and are misleading. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
In order to import the openjdk-jfx project correctly in the different IDEs, the dependencies should be clearly defined. At the moment the project is working in IDEA and Netbeans correctly.
Currently, Netbeans has an issue open with gradle and jigsaw (https://issues.apache.org/jira/browse/NETBEANS-2004 and kelemen/netbeans-gradle-project#417).
So the only IDE working 100% correctly is IDEA for importing the openjdk-jfx project.
https://bugs.openjdk.java.net/browse/JDK-8220222